-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Filter out bundle versions lower than installed #711
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
b4301b5
to
156420e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #711 +/- ##
==========================================
+ Coverage 63.50% 68.15% +4.65%
==========================================
Files 22 22
Lines 1403 1429 +26
==========================================
+ Hits 891 974 +83
+ Misses 459 390 -69
- Partials 53 65 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
5e80cd5
to
05db735
Compare
@@ -31,6 +31,19 @@ func InMastermindsSemverRange(semverRange *mmsemver.Constraints) Predicate[catal | |||
} | |||
} | |||
|
|||
func HigherBundleVersion(currentVersion *bsemver.Version) Predicate[catalogmetadata.Bundle] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good start, but the end state needs to be the same as the current behavior of the ClusterExtension, which is one of:
- Semver semantics (if the "force semver" feature gate is enabled)
- Replaces, skips, skipRange semantics (which finds other bundles that have skips, replaces, skipRange that include the currently installed bundle)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I'm not sure ClusterExtension handles skips and skipRange for the second case. But it needs to there too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joelanford Coming back to this after a v0 distraction and had a follow-up question here: for the second case, are we looking for bundles that do all three (replaces and skips and skipRange) or any one of the three?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any, and then of all the matches, choose the one with the highest semver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @joelanford ! I was just about finished adding the feature gating and replaces/skips/skipsRange then realized it was going to make the PR significantly larger (mostly due to tests). How do you feel about me doing that as a follow-up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on wrapping this and capturing a follow-up issue.
I've implemented this for ClusterExtension here: #743
Given the recent change roadmap updates, I think we should try to wrap up work on Extension API for now, and capture a "current state of the world" of the Extension API and behavior.
func hasKappApis(config *rest.Config) (bool, error) { | ||
discoveryClient, err := discovery.NewDiscoveryClientForConfig(config) | ||
if err != nil { | ||
return false, fmt.Errorf("creating discovery client: %v", err) | ||
} | ||
apiResourceList, err := discoveryClient.ServerResourcesForGroupVersion(carvelv1alpha1.SchemeGroupVersion.String()) | ||
if err != nil && !errors.IsNotFound(err) { | ||
return false, fmt.Errorf("listing resource APIs: %v", err) | ||
} | ||
|
||
if apiResourceList == nil { | ||
return false, nil | ||
} | ||
|
||
for _, resource := range apiResourceList.APIResources { | ||
if resource.Kind == "App" { | ||
return true, nil | ||
} | ||
} | ||
return false, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was taken from #690 , in which Varsha explained that the check is not needed due to the feature gate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, why is it in yours? 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That PR is undergoing a refactor to combine the tests into a test loop, which is likely going to take some time given its relative priority. This particular change is valuable though since it made doing these tests much easier.
05db735
to
aec5d08
Compare
if currentVersion == nil { | ||
return false | ||
} | ||
bundleVersion, err := bundle.Version() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR, just a though. I wonder if we should validate the bundle version on creation/ingress rather than on egress (i.e. bundle.Version() shouldn't return an error). So, a bundle object is always validated and correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had this thought too. If it didn't break any assumptions, it would be nice to get something into the declcfg.Bundle
type for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that makes great sense and also would make it a lot easier to grab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll create a discussion upstream - see what ppl think - or would a ticket suffice?
Properties: []property.Property{ | ||
{ | ||
Type: property.TypePackage, | ||
Value: json.RawMessage(`{"packageName": "package1", "version": "1.0.0"}`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. I guess we'd need some sort of factory is we want to validate on creation...
Adds an annotation to the App created for each extension indicating the bundle version, which can then be used on future reconciles to filter out older versions. This can be overridden by setting the UpgradeConstraintPolicy to 'Ignore'. Co-authored-by: Varsha Prasad Narsing <varshaprasad96@gmail.com> Signed-off-by: dtfranz <dfranz@redhat.com>
aec5d08
to
92ad69a
Compare
Thank you for reviewing @perdasilva , I've updated per your comments. Please take another look when you can and let me know if you see anything else! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chef's kiss! Thanks mate!!
49be998
Description
Adds an annotation to the App created for each extension indicating the bundle version, which can then be used on future reconciles to filter out older versions. This can be overridden by setting the Extension's
UpgradeConstraintPolicy
toIgnore
.Closes #654
This PR borrows a lot of stuff from @varshaprasad96 's PR #690 so I've added her as co-author :)
Reviewer Checklist